Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use reccmp as a python requirement #1116

Merged
merged 2 commits into from
Oct 26, 2024
Merged

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Oct 24, 2024

You can install all tools, and ncc's requirements, by running:

pip install -r tools/requirements.txt

@madebr madebr marked this pull request as draft October 24, 2024 18:31
@madebr madebr marked this pull request as ready for review October 24, 2024 18:52
@foxtacles
Copy link
Member

You can install all tools, and ncc's requirements, by running:

python -r tools/requirements.txt

You mean pip install -r tools/requirements.txt, right?

Documentation looks good to me - just set up everything locally as described and it worked out of the box. @disinvite @jonschz if you could check/test as well and confirm everything looks good to you this should be ready to merge.

@jonschz
Copy link
Collaborator

jonschz commented Oct 24, 2024

Looks great overall! I ran into one minor issue which we may need to agree on:

  • At the moment, all registered original binaries need to be present for the code to execute at all (e.g. I can't run datacmp on LEGO1 if CONFIG.EXE isn't present). At a glance, this looks like something that can (and probably should) be changed. I don't necessarily consider that to be a blocker, but I'm happy to hear your opinions as well. -> fixed by Don't make built target available if it is missing original path, recompiled path, or recompiled pdb path reccmp#9
  • Currently, BETA10 is not part of the (commited) reccmp-project.yml. If we add it there before the first point is addressed, all developers (and probably the CI) need BETA10.dll to be present so any reccmp scripts can be run. If we don't add it to the file, developers that want to use BETA10 for local development have to change reccmp-project.yml locally but not commit it.
  • Ghidra imports do not work yet, but since we don't really depend on them and I intend to fix them soon anyway, I don't consider that to be a blocker. -> see Fix the Ghidra import reccmp#2

@jonschz
Copy link
Collaborator

jonschz commented Oct 25, 2024

Thanks for the quick fix @madebr ! Maybe we want to add the following to the project yaml?

  BETA10:
    filename: BETA10.DLL
    source-root: LEGO1
    hash:
      sha256: d91435a40fa31f405fba33b03bd3bd40dcd4ca36ccf8ef6162c6c5ca0d7190e7

@madebr
Copy link
Contributor Author

madebr commented Oct 25, 2024

Thanks for the quick fix @madebr ! Maybe we want to add the following to the project yaml?

Done.
About the warning messages, this situation will improve once we dive into isledecomp/reccmp#8

@disinvite
Copy link
Collaborator

Everything that I've checked looks good. I noticed that the roadmap tool still expects the original four args instead of using the project file, but it's still perfectly usable. I added isledecomp/reccmp#10 for this.

The tools other than reccmp-reccmp have more logging messages than they did before, but this also isn't a critical problem. The main impact would be to the progress report diff, but this is run by reccmp, so no change. @jonschz already put in isledecomp/reccmp#5 so we can discuss over there.

Would it be worth adding a note to the main README.md to point at the readme under /tools?

@jonschz
Copy link
Collaborator

jonschz commented Oct 26, 2024

Would it be worth adding a note to the main README.md to point at the readme under /tools?

It is linked under CONTRIBUTING.md, which I feel is sufficient.

However, I noticed that the README under /tools still has a lot of duplication with the README in the reccmp repo (e.g. the explanation for the different annotation types). I think this can be cleaned up later, though.

@foxtacles foxtacles merged commit 0cb753e into isledecomp:master Oct 26, 2024
11 checks passed
@madebr madebr deleted the use-reccmp-fork branch October 26, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants